doc,tools: get altDocs versions from CHANGELOG.md #27661

richardlau · 2019-05-12T17:14:09Z

Parse CHANGELOG.md for versions of Node.js used by the documentation
feature View another version so that we don't have to manually update
the list when we cut a new version or transition a release to LTS.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

BridgeAR

The code itself LGTM while I would prefer not to use a fixture. That could be done by
a) not reading the whole changelog entry but only a small top part of the file
b) by checking that the current output is the "minimum" expectation instead of the absolute truth and that there could be more versions and more lts entries.

That way it would be easier to detect changes that might break this (e.g. parts of the file structure would be changed or old entries would be moved). That is not as likely but it still seems a possibility.

But I'm also fine with the code as is. This is definitely an improvement over the current situation!

richardlau · 2019-05-12T18:26:49Z

I've just realised this isn't going to work as expected outside of master because we don't port the CHANGELOG.md changes backwards (e.g. https://github.com/nodejs/node/blob/v8.x/CHANGELOG.md doesn't know of any version greater than 8). We do port the altDocs changes, e.g. for Node.js 8:

node/tools/doc/html.js

Lines 170 to 181 in 07458ba

    
           const versions = [ 
        
             { num: '11.x' }, 
        
             { num: '10.x', lts: true }, 
        
             { num: '9.x' }, 
        
             { num: '8.x', lts: true }, 
        
             { num: '7.x' }, 
        
             { num: '6.x', lts: true }, 
        
             { num: '5.x' }, 
        
             { num: '4.x', lts: true }, 
        
             { num: '0.12.x' }, 
        
             { num: '0.10.x' } 
        
           ];

which allows docs for, e.g. Node.js 8, to point to newer versions, e.g. Node.js 10.

richardlau · 2019-05-12T18:33:10Z

I've just realised this isn't going to work as expected outside of master because we don't port the CHANGELOG.md changes backwards (e.g. https://github.com/nodejs/node/blob/v8.x/CHANGELOG.md doesn't know of any version greater than 8). We do port the altDocs changes, e.g. for Node.js 8:

node/tools/doc/html.js

Lines 170 to 181 in 07458ba

const versions = [

{ num: '11.x' },

{ num: '10.x', lts: true },

{ num: '9.x' },

{ num: '8.x', lts: true },

{ num: '7.x' },

{ num: '6.x', lts: true },

{ num: '5.x' },

{ num: '4.x', lts: true },

{ num: '0.12.x' },

{ num: '0.10.x' }

];

which allows docs for, e.g. Node.js 8, to point to newer versions, e.g. Node.js 10.

And I've just realised that the table for Node.js 8 doesn't match the table in master:

node/tools/doc/html.js

Lines 396 to 407 in b230833

    
           const versions = [ 
        
             { num: '11.x' }, 
        
             { num: '10.x', lts: true }, 
        
             { num: '9.x' }, 
        
             { num: '8.x', lts: true }, 
        
             { num: '7.x' }, 
        
             { num: '6.x', lts: true }, 
        
             { num: '5.x' }, 
        
             { num: '4.x' }, 
        
             { num: '0.12.x' }, 
        
             { num: '0.10.x' }, 
        
           ];

because we haven't backported #20904 (which removed the LTS label from Node.js 4).

BridgeAR · 2019-05-12T18:39:48Z

@richardlau we could check the file in the repository instead. That's not a great solution but it would at least always work in case there's a network connection. That way it's always up to date.

richardlau · 2019-05-12T18:46:58Z

@richardlau we could check the file in the repository instead. That's not a great solution but it would at least always work in case there's a network connection. That way it's always up to date.

Yeah, less than ideal but probably okay? I'll do that later (after I've eaten).

I've pushed a change to remove the test fixture and do sanity checks on the versions (e.g. LTS releases are never odd numbered releases) rather than a deepStrictEqual assertion.

richardlau · 2019-05-12T18:52:19Z

@BridgeAR Quick thought -- If we're going to need a network connection we might be better off reading schedule.json from the Release repository (with some checks that the version start date isn't in the future to avoid picking up unreleased versions, e.g. v13) rather than relying on the changelog?

BridgeAR · 2019-05-12T18:59:51Z

@richardlau I thought about that as well and it is indeed the perfect data. The reason why I still prefer the changelog approach right now is that it's another repository and we update that file manually. We did lack behind before (just a bit but still) and might forget to update that file at times while that would not happen with the changelogs.

richardlau · 2019-05-13T01:30:41Z

@richardlau I thought about that as well and it is indeed the perfect data. The reason why I still prefer the changelog approach right now is that it's another repository and we update that file manually. We did lack behind before (just a bit but still) and might forget to update that file at times while that would not happen with the changelogs.

We did forget to update the list of versions in the changelog, see #27414 (this step is in the release process docs being proposed in #25497). I've kept with parsing the changelog for now (albeit the code now fetches the version from the master branch) since I'd already written the parsing regular expressions.

Since the code now fetches from the master branch building the docs and the newly added test will make an Internet request.

cc @nodejs/documentation

BridgeAR · 2019-05-13T09:06:25Z

@richardlau we could also just parse the filenames instead of reading the entries from the main changelog.md. I doubt that we'll ever forget a to add such entry. But I'm actually happy with either implementation as long as it automates the process.

The test itself should probably be moved to test/internet.

richardlau · 2019-05-13T14:05:23Z

@richardlau we could also just parse the filenames instead of reading the entries from the main changelog.md. I doubt that we'll ever forget a to add such entry. But I'm actually happy with either implementation as long as it automates the process.

The test itself should probably be moved to test/internet.

@BridgeAR We won't be able to determine LTS status by just parsing filenames.

I've moved the test to test/internet and also added a test for the previous semver major (i.e. if master is v13.0.0-pre it checks for 12.x). This additional test should catch the cases where we forget to update the main changelog (i.e. when 13.x is released and master moves to v14.0.0-pre the test will fail if Node.js 13 is not added to the main changelog).

test/internet/test-doctool-versions.js

nodejs-github-bot · 2019-05-13T22:13:42Z

CI: https://ci.nodejs.org/job/node-test-pull-request/23097/

ChALkeR · 2019-05-14T00:24:43Z

#27658 (comment) is also relevant here -- the LTS labels in current master don't seem correct.

4.x and 6.x are both EOL (and both were LTS previously), so they should have identical labelling (either both have LTS label or both not have it, or both have an EOL label).

Refs: #20904 nodejs/Release#440

richardlau · 2019-05-14T00:46:18Z

#27658 (comment) is also relevant here -- the LTS labels in current master don't seem correct.

4.x and 6.x are both EOL (and both were LTS previously), so they should have identical labelling (either both have LTS label or both not have it, or both have an EOL label).

Refs: #20904 nodejs/Release#440

I pushed an update to #27658 that makes the LTS status consistent (both 4 and 6 no longer have it). Once the changelog is consistent this PR will make the LTS labels in the "View another version" drop down in the built docs consistent automatically (provided we keep the changelog accurate).

nodejs-github-bot · 2019-05-16T15:41:10Z

CI: https://ci.nodejs.org/job/node-test-pull-request/23145/

addaleax · 2019-05-26T14:54:52Z

@richardlau This is ready, right? Do you want to go ahead and land this?

richardlau · 2019-05-26T20:40:59Z

@richardlau This is ready, right? Do you want to go ahead and land this?

I think it was. It’s a long weekend here in UK (public holiday on Monday) so it’ll be Tuesday at the earliest before I can land this.

Parse `CHANGELOG.md` for versions of Node.js used by the documentation feature `View another version` so that we don't have to manually update the list when we cut a new version or transition a release to LTS.

richardlau · 2019-05-29T11:33:54Z

@richardlau This is ready, right? Do you want to go ahead and land this?

I think it was. It’s a long weekend here in UK (public holiday on Monday) so it’ll be Tuesday at the earliest before I can land this.

node-core-utils says this isn't ready to land because changes were pushed after the review. Taken the opportunity to rebase and squash the fixup commits and address @Trott 's suggested changes to avoid the use of sanity (as in sanity checks) in the comments.

nodejs-github-bot · 2019-05-29T11:34:42Z

CI: https://ci.nodejs.org/job/node-test-pull-request/23452/

nodejs-github-bot · 2019-05-29T12:26:38Z

CI: https://ci.nodejs.org/job/node-test-pull-request/23453/

Trott · 2019-05-30T06:16:04Z

Still LGTM.

richardlau · 2019-05-30T17:23:54Z

Still LGTM.

I'd assumed (perhaps naively) that this LGTM should have satisfied node-core-utils, but it does not:

-bash-4.2$ git-node land 27661
✔  Done loading data for nodejs/node/pull/27661
----------------------------------- PR info ------------------------------------
Title      doc,tools: get altDocs versions from CHANGELOG.md (#27661)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     richardlau:autoaltdocs -> nodejs:master
Labels     doc, review wanted, tools
Commits    1
 - doc,tools: get altDocs versions from CHANGELOG.md
Committers 1
 - Richard Lau <[email protected]>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/27661
Reviewed-By: Rich Trott <[email protected]>
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last review:
   ⚠  - doc,tools: get altDocs versions from CHANGELOG.md
   ℹ  Last Full PR CI on 2019-05-29T12:26:38Z: https://ci.nodejs.org/job/node-test-pull-request/23453/
   ℹ  This PR was created on Sun, 12 May 2019 17:14:09 GMT
   ✔  Approvals: 1
   ✔  - Rich Trott (@Trott) (TSC): https://github.com/nodejs/node/pull/27661#pullrequestreview-236953915
--------------------------------------------------------------------------------
? This PR is not ready to land, do you want to continue?

There's a couple of reasons for this:

The comment in question doesn't get recognized by the regular expression https://github.com/nodejs/node-core-utils/blob/1e9daeb15475efdfea4a418686395299ad892149/lib/reviews.js#L7. I'd be a little wary relaxing the regexp as, for example, I'm not sure we'd want to treat the LGTM in doc,tools: get altDocs versions from CHANGELOG.md #27661 (review) as an approval.
As far as I can tell the code that checks commits since the last review only looks at reviews and not LGTM's (assuming they were detected) in the comments.

Parse `CHANGELOG.md` for versions of Node.js used by the documentation feature `View another version` so that we don't have to manually update the list when we cut a new version or transition a release to LTS. PR-URL: nodejs#27661 Reviewed-By: Rich Trott <[email protected]>

Trott · 2019-06-01T08:18:03Z

Landed in 4ec6135

Parse `CHANGELOG.md` for versions of Node.js used by the documentation feature `View another version` so that we don't have to manually update the list when we cut a new version or transition a release to LTS. PR-URL: #27661 Reviewed-By: Rich Trott <[email protected]>

Parse `CHANGELOG.md` for versions of Node.js used by the documentation feature `View another version` so that we don't have to manually update the list when we cut a new version or transition a release to LTS. Backport-PR-URL: nodejs#32642 PR-URL: nodejs#27661 Reviewed-By: Rich Trott <[email protected]>

Parse `CHANGELOG.md` for versions of Node.js used by the documentation feature `View another version` so that we don't have to manually update the list when we cut a new version or transition a release to LTS. Backport-PR-URL: #32642 PR-URL: #27661 Reviewed-By: Rich Trott <[email protected]>

This comment has been minimized.

Sign in to view

nodejs-github-bot added doc Issues and PRs related to the documentations. tools Issues and PRs related to the tools directory. labels May 12, 2019

richardlau mentioned this pull request May 12, 2019

tools: add 12.x to alternative docs versions #27658

Closed

2 tasks

This comment has been minimized.

Sign in to view

richardlau mentioned this pull request May 12, 2019

meta: update release guide with notes for major releases #25497

Closed

4 tasks

BridgeAR reviewed May 12, 2019

View reviewed changes

richardlau added the wip Issues and PRs that are still a work in progress. label May 12, 2019

richardlau removed the wip Issues and PRs that are still a work in progress. label May 13, 2019

This comment has been minimized.

Sign in to view

mihalskiy approved these changes May 13, 2019

View reviewed changes

richardlau force-pushed the autoaltdocs branch from 77d3297 to 3daa27c Compare May 13, 2019 18:30

Trott reviewed May 13, 2019

View reviewed changes

test/internet/test-doctool-versions.js Outdated Show resolved Hide resolved

Trott reviewed May 13, 2019

View reviewed changes

test/internet/test-doctool-versions.js Outdated Show resolved Hide resolved

Trott approved these changes May 13, 2019

View reviewed changes

Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 13, 2019

richardlau force-pushed the autoaltdocs branch from 33e0e81 to 11f6ca9 Compare May 16, 2019 15:40

doc,tools: get altDocs versions from CHANGELOG.md

e0abeb8

Parse `CHANGELOG.md` for versions of Node.js used by the documentation feature `View another version` so that we don't have to manually update the list when we cut a new version or transition a release to LTS.

richardlau force-pushed the autoaltdocs branch from 11f6ca9 to e0abeb8 Compare May 29, 2019 11:28

richardlau added review wanted PRs that need reviews. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels May 29, 2019

Trott approved these changes Jun 1, 2019

View reviewed changes

Trott closed this Jun 1, 2019

targos mentioned this pull request Jun 3, 2019

v12.4.0 release proposal #28040

Merged

richardlau mentioned this pull request Oct 10, 2019

test/doctool/test-doctool-html requires internet connection? #29918

Closed

richardlau deleted the autoaltdocs branch November 2, 2019 08:49

richardlau mentioned this pull request Apr 3, 2020

[v10.x backport] doc,tools: get altDocs versions from CHANGELOG.md #32642

Closed

3 tasks

BethGriggs mentioned this pull request Apr 7, 2020

v10.20.0 proposal #31984

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

doc,tools: get altDocs versions from CHANGELOG.md #27661

doc,tools: get altDocs versions from CHANGELOG.md #27661

richardlau commented May 12, 2019

This comment has been minimized.

This comment has been minimized.

BridgeAR left a comment

richardlau commented May 12, 2019 •

edited

Loading

richardlau commented May 12, 2019

BridgeAR commented May 12, 2019

richardlau commented May 12, 2019

richardlau commented May 12, 2019

BridgeAR commented May 12, 2019

This comment has been minimized.

richardlau commented May 13, 2019

BridgeAR commented May 13, 2019

richardlau commented May 13, 2019

nodejs-github-bot commented May 13, 2019

ChALkeR commented May 14, 2019 •

edited

Loading

richardlau commented May 14, 2019

nodejs-github-bot commented May 16, 2019

addaleax commented May 26, 2019

richardlau commented May 26, 2019

richardlau commented May 29, 2019

nodejs-github-bot commented May 29, 2019

nodejs-github-bot commented May 29, 2019

Trott commented May 30, 2019

richardlau commented May 30, 2019

Trott commented Jun 1, 2019

doc,tools: get altDocs versions from CHANGELOG.md #27661

doc,tools: get altDocs versions from CHANGELOG.md #27661

Conversation

richardlau commented May 12, 2019

Checklist

This comment has been minimized.

This comment has been minimized.

BridgeAR left a comment

Choose a reason for hiding this comment

richardlau commented May 12, 2019 • edited Loading

richardlau commented May 12, 2019

BridgeAR commented May 12, 2019

richardlau commented May 12, 2019

richardlau commented May 12, 2019

BridgeAR commented May 12, 2019

This comment has been minimized.

richardlau commented May 13, 2019

BridgeAR commented May 13, 2019

richardlau commented May 13, 2019

nodejs-github-bot commented May 13, 2019

ChALkeR commented May 14, 2019 • edited Loading

richardlau commented May 14, 2019

nodejs-github-bot commented May 16, 2019

addaleax commented May 26, 2019

richardlau commented May 26, 2019

richardlau commented May 29, 2019

nodejs-github-bot commented May 29, 2019

nodejs-github-bot commented May 29, 2019

Trott commented May 30, 2019

richardlau commented May 30, 2019

Trott commented Jun 1, 2019

richardlau commented May 12, 2019 •

edited

Loading

ChALkeR commented May 14, 2019 •

edited

Loading